#1813 - Fix createObject with Google GenAI thinking fails: answer JSON never selected from multi-part response - #1814
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses Google GenAI “thinking” multi-part responses where the final structured JSON answer can arrive in a later (non-thought) generation, and updates Spring AI message handling to select the correct generation for structured output.
Changes:
- Update Spring AI response resolution to select non-thought (answer) generations instead of always using
ChatResponse.result, while preserving tool calls + merged metadata. - Preserve provider metadata (e.g.,
thoughtSignatures) during Spring AI → Embabel message conversion even when there are no tool calls. - Add unit + integration regression tests covering multi-generation thought/answer selection and metadata preservation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| embabel-agent-autoconfigure/models/embabel-agent-google-genai-autoconfigure/src/test/kotlin/com/embabel/agent/config/models/googlegenai/GoogleGenAiOptionsConverterTest.kt | Adds tests around includeThoughts behavior for Thinking.NONE vs thinking == null. |
| embabel-agent-autoconfigure/models/embabel-agent-google-genai-autoconfigure/src/test/kotlin/com/embabel/agent/config/models/googlegenai/GoogleGenAiChatIntegrationIT.kt | Adds live regression ITs for multi-part thinking + structured output paths (including tools). |
| embabel-agent-api/src/test/kotlin/com/embabel/agent/spi/support/springai/SpringAiLlmMessageSenderTest.kt | Adds regression tests for multi-generation resolution (thought vs answer, metadata/tool-calls). |
| embabel-agent-api/src/test/kotlin/com/embabel/agent/spi/support/springai/MessageConversionTest.kt | Adds tests ensuring provider metadata is preserved through message conversion. |
| embabel-agent-api/src/main/kotlin/com/embabel/agent/spi/support/springai/SpringAiLlmMessageSender.kt | Implements multi-generation resolution (isThought filtering + safer selection) for structured output. |
| embabel-agent-api/src/main/kotlin/com/embabel/agent/spi/support/springai/messageConverters.kt | Preserves provider metadata by converting to AssistantMessageWithToolCalls even when tool calls are absent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…N never selected from multi-part response Select non-thought answer generations for structured output instead of using the first/thought generation or concatenating alternative JSON candidates. Preserve provider metadata, including thoughtSignatures, on no-tool assistant responses by using metadata-capable assistant messages. Add deterministic regression coverage for multi-generation Google GenAI thinking responses, tool-call continuation, metadata preservation, and negative structured-output cases. Add live Google GenAI IT coverage and options converter tests for includeThoughts behavior. Signed-off-by: Slava Imeshev <imeshev@yahoo.com>
…sertions verify answer-only selection (and explicitly avoid requiring a thought prefix). Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Slava Imeshev <imeshev@yahoo.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Slava Imeshev <imeshev@yahoo.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Slava Imeshev <imeshev@yahoo.com>
4f8ce69 to
432990c
Compare
|
|
@kenkc - could you please review the PR when you have a chance? Thank you |
|
@simeshev - please confirm the same behavior in 1.0.0, and it's not something being introduced due to the Spring AI upgrade, thanks |
My plan is to land this fix in 1.5.0 because that's the surface where the issue was found. After that research the applicability to 1.0.x, and if it applies, backport from 1.5.0 to 1.0.x via a separate issue and and a PR. |
|
@@kenkc - could you please advise when you would be able to review? Thank you |
| val toolCalls = this.toolCalls | ||
| val content = this.text ?: "" | ||
| val metadata = this.metadata ?: emptyMap() | ||
| val hasProviderMetadata = metadata.keys.any { it != "messageType" } |
There was a problem hiding this comment.
{ it != "messageType" } ==> specific to GEN AI provider?
| * This ensures we don't lose valuable content (text or tool calls) from any generation. | ||
| * | ||
| * @return A merged AssistantMessage with all tool calls and text, or null if no tool calls found | ||
| * 1. Collect tool calls and metadata from every generation. |
|
@igordayen - will do tomorrow. Family matters occupied all my time. |
| val toolCalls = this.toolCalls | ||
| val content = this.text ?: "" | ||
| val metadata = this.metadata ?: emptyMap() | ||
| val hasProviderMetadata = metadata.keys.any { it != "messageType" } |
There was a problem hiding this comment.
Could do with a comment to explain how this resolves to having provider metadata.
| .filterNot { isThoughtGeneration(it) } | ||
| .mapNotNull { it.text?.takeIf { text -> text.isNotBlank() } } | ||
| if (nonThoughtTexts.isNotEmpty()) { | ||
| return nonThoughtTexts.first() |
There was a problem hiding this comment.
The previous code returned text from all generations. Here you are just returning the first. I understand the reason from the function comment, however is this specific to the Google GenAI implementation? How does this affect responses from other providers?
| * Spring AI's Google GenAI adapter uses Boolean `true`; trimmed string values are accepted | ||
| * so metadata copied through less strongly typed paths is still filtered correctly. | ||
| */ | ||
| private fun isThoughtGeneration( |
There was a problem hiding this comment.
I would be concerned about putting provider-specific metadata handling like isThought directly into SpringAiLlmMessageSender. SpringAiLlmMessageSender is a shared Spring AI integration layer, so once we start coding Google-specific response semantics in here we are on a slippery slope. It would be better if we could make response resolution provider-pluggable in some manner. The Spring team have yet to deal with this themselves as outlined here: spring-projects/spring-ai#4269
Apologies - added comments last week but forgot to submit the review. |
No description provided.